-
Notifications
You must be signed in to change notification settings - Fork 0
support K8s service account for auth #5
base: master
Are you sure you want to change the base?
Conversation
.. mostly stolen from Jack Newton's old PR
// .is_err() { | ||
// return Err(Error::); | ||
// // return Err(String::from("Kubernetes authentication failed")); | ||
// }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this error handling not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure how to add it properly..
read_to_string?
is not possible (perhaps, after Rust language changes)
and I could not find a usage Error
class.. I guess, new one needs to be created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey 👋 this project is open source, and I apparently have nothing better to do with my time even though I don't work at Avvo anymore.
let mut file = File::open("/var/run/secrets/kubernetes.io/serviceaccount/token").expect("Unable to open"); | ||
let mut jwt = String::new(); | ||
file.read_to_string(&mut jwt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both File::open
and File::read_to_string
can return a std::io::Error
, to be able to use the ?
operator you need to be able to convert a std::io::Error
to the Error
type this function returns (defined up on line 87).
To be able to do that conversion you need to implement the std::convert::From
trait. Currently the way Error
is defined it holds a ClientError
, but you've got a std::io::Error
, so you probably want it to also be able to hold a ClientError
or a std::io::Error
. You would do this by changing it to an enum, like:
pub enum Error {
ClientError(ClientError),
IoError(std::io::Error),
}
You'd then need to update the various trait implementations for ClientError
to now be able to cope with it being an enum, and finally implement the From
trait. Check client_error.rs
for examples.
Ignoring the error from File::read_to_string
will mean that if it fails you'll end up with an empty jwt
, and things will probably break later down the line in confusing ways.
Using expect(...)
on the error from File::open
will mean the program will immediately crash with an ugly error, instead of passing the error back up to any error handing that may exist in the caller.
use std::{fmt, str::FromStr}; | ||
|
||
use std::fs::File; | ||
use std::io::Read; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can combine these into a single statement like use std::{io::Read, fmt, fs::File, str::FromStr};
let role: String = service.to_string(); | ||
vault.kubernetes_auth(role)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
service
is already a String
, what you want here is to either give kubernetes_auth
a copy of service, like vault.kubernetes_auth(service.clone())?;
, or lend it a &str
reference to service
like vault.kubernetes_auth(&service)?;
.
The second option requires a small change to kubernetes_auth
(explained in later comments), but makes more sense in Rust, and avoids a needless copy.
In Rust when you give a function a value it is moved into that function, and you can't get it back, but using &
lets you lend a value to a function in a way that you do get it back.
@@ -135,6 +144,20 @@ impl Client { | |||
Ok(()) | |||
} | |||
|
|||
pub fn kubernetes_auth(&mut self, role: String) -> Result<(), Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to take an owned copy of the role
argument, it would work fine with just a reference, like pub fn kubernetes_auth(&mut self, role:&str) -> Result<(), Error> {
. That will require a tweak to AuthKubernetesRequest
, see the comment on AuthKubernetesRequest
.
pub struct AuthKubernetesRequest { | ||
pub jwt: String, | ||
pub role: String | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be able to just take string references like:
#[derive(Serialize)]
pub struct AuthKubernetesRequest<'a> {
pub jwt: &'a str,
pub role: &'a str,
}
You'll with that change probably need to create an instance like:
AuthKubernetesRequest { jwt: &jwt, role }
as you'll have an owned String
, but it wants a &str
reference (role
will already be a &str
reference if you follow my other comments).
// // return Err(String::from("Kubernetes authentication failed")); | ||
// }; | ||
let request = AuthKubernetesRequest { jwt, role }; | ||
let response: AuthResponseWrapper = self.post(&format!("auth/kubernetes/login"), &request)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to use format here, as you're not adding anything to the string, just self.post("auth/kubernetes/login", &request)?;
should do.
.. mostly stolen from Jack Newton's old PR #3
/cc @jnewton-avvo